Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

installation: CNF Installation (7) Verify new installation process #2177

Merged
merged 2 commits into from
Nov 28, 2024
Merged

installation: CNF Installation (7) Verify new installation process #2177

merged 2 commits into from
Nov 28, 2024

Conversation

svteb
Copy link
Collaborator

@svteb svteb commented Nov 12, 2024 •
edited
Loading

Description

  • New setup spec tests were introduced to verify that the installation method from [Feature] CNF Installation (6), Prepare new installation process #2161 works as intended.
  • Three distinct issues were discovered and resolved:
    • Helm directories with variable depth levels could not be discovered after initialization (resolved through File.basename).
    • Configs that weren't named cnf-testsuite.yml could not be discovered after initialization (resolved by changing the name internally when copying the file).
    • Existence of manifest folder was not checked (resolved by adding an if check).
  • Additionally a common point of failure was added with a failure message that was previously not present.
  • The issue [Feature] Allow to specify deployment order in cnfs that have multiple deployments #2176 was also discovered during attempted deployment of sample-cnfs/sample-elk-stack which was also added in this commit and is to be used in future spec test.

For reviewers

The "validate_config" and "dummy-tag" spec tests are intentionally failing, this is because we are still blocking multiple deployments in one CNF and they are thus considered invalid, these spec tests will start passing again after #2171. A lot of new sample cnfs were added to test the new installation method making this commit look quite bloated, when reviewing just ignore whatever files that start with sample-cnfs (except for the new configs).

Issues:

Refs: #2168

How has this been tested:

  • Covered by existing integration testing
  • Added integration testing to cover
  • Verified all A/C passes
    • develop
    • master
    • tag/other branch
  • Test environment
    • Shared Packet K8s cluster
    • New Packet K8s cluster
    • Kind cluster
  • Have not tested

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist:

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • No updates required.

Code Review

  • Does the test handle fatal exceptions, ie. rescue block

Issue

  • Tasks in issue are checked off

kosstennbl
kosstennbl requested changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@kosstennbl kosstennbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't checked the sample CNFs, but i suppose that they should work fine,
Two notes:

src/tasks/utils/cnf_installation/config_versions/config_v2.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/install_common.cr Outdated Show resolved Hide resolved
@svteb svteb force-pushed the new-installation-verification branch 2 times, most recently from df63e7a to 7621efb Compare November 14, 2024 09:30
kosstennbl
kosstennbl approved these changes Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@kosstennbl kosstennbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
Dummy-tag specs are preparation for step 8 and are meant to fail.
Validate_config spec is failing because of addition of multiple-deployment CNF for dummy-tag tests whilst multiple deployments are not supported yet for regular functionality of testsuite. So, CI failures are expected and don't point to any actual problems.

martin-mat
martin-mat approved these changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@martin-mat martin-mat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@collivier collivier force-pushed the new-installation-verification branch from 7621efb to f06c51f Compare November 27, 2024 11:52
collivier
collivier requested changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@collivier collivier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else lgtm

sample-cnfs/sample_multiple_deployments/memcached/charts/common/templates/_utils.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/memcached/charts/common/templates/_utils.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_compatibility.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_resources.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_resources.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_resources.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_resources.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_resources.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_utils.tpl Outdated Show resolved Hide resolved
sample-cnfs/sample_multiple_deployments/nginx/charts/common/templates/_utils.tpl Outdated Show resolved Hide resolved
@svteb
Feat: Add spec tests for new installation method and fix minor bugs
8c37d23 
Refs: #2168
- New setup spec tests were introduced to verify that the installation
  method from #2161 works as intended.
- Three distinct issues were discovered and resolved:
  * Helm directories with variable depth levels could not be discovered
  after initialization (resolved through File.basename).
  * Configs that weren't named cnf-testsuite.yml could not
  be discovered after initialization (resolved by changing the name
  internally when copying the file).
  * Existence of manifest folder was not checked (resolved by adding an
  if check).
- Additionally a common point of failure was added with a failure
  message that was previously not present.
- The issue #2176 was also discovered during attempted deployment of
  sample-cnfs/sample-elk-stack which was also added in this commit and
  is to be used in future spec test.

Signed-off-by: svteb <[email protected]>
@svteb svteb force-pushed the new-installation-verification branch from f06c51f to 8c37d23 Compare November 28, 2024 10:30
@collivier collivier merged commit 3261537 into main Nov 28, 2024
165 of 172 checks passed
@collivier collivier deleted the new-installation-verification branch November 28, 2024 14:37
@kosstennbl kosstennbl mentioned this pull request Nov 29, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kosstennbl kosstennbl kosstennbl approved these changes

@martin-mat martin-mat martin-mat approved these changes

@collivier collivier collivier approved these changes

@denverwilliams denverwilliams Awaiting requested review from denverwilliams denverwilliams is a code owner

@agentpoyo agentpoyo Awaiting requested review from agentpoyo agentpoyo is a code owner

@Smitholi67 Smitholi67 Awaiting requested review from Smitholi67

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@svteb @collivier @martin-mat @kosstennbl